keep-desktop/src/app.rs (1)

307-317: Normalize/dedupe/cap loaded relays to match runtime constraints

filter_valid_relays currently only validates. Consider normalizing, deduplicating, and enforcing MAX_RELAYS during load to keep disk-loaded behavior consistent with add-relay behavior.

♻️ Suggested patch

 fn filter_valid_relays(urls: Vec<String>) -> Vec<String> {
-    urls.into_iter()
-        .filter(|url| match validate_relay_url(url) {
-            Ok(()) => true,
-            Err(e) => {
-                tracing::warn!(url, "Dropping invalid relay loaded from disk: {e}");
-                false
-            }
-        })
-        .collect()
+    let mut out = Vec::new();
+    for raw in urls {
+        let normalized = normalize_relay_url(&raw);
+        match validate_relay_url(&normalized) {
+            Ok(()) => {
+                if !out.contains(&normalized) {
+                    out.push(normalized);
+                    if out.len() >= MAX_RELAYS {
+                        break;
+                    }
+                }
+            }
+            Err(e) => {
+                tracing::warn!(url = %raw, "Dropping invalid relay loaded from disk: {e}");
+            }
+        }
+    }
+    out
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-desktop/src/app.rs` around lines 307 - 317, filter_valid_relays
currently only validates but doesn't normalize, dedupe, or cap loaded relays to
the same runtime rules used by the add-relay flow; update filter_valid_relays
to: normalize each url (e.g., lowercasing host, removing default ports) before
validation via validate_relay_url, deduplicate while preserving order, and
enforce the MAX_RELAYS limit (same policy as add-relay) by truncating the final
Vec to MAX_RELAYS; ensure any dropped/duplicate entries are traced with
tracing::warn similar to current behavior so behavior matches runtime add-relay
constraints.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-core/src/relay.rs (1)

154-165: Past concern about fec0::/10 has been addressed.

Line 161 now includes (segments[0] & 0xffc0) == 0xfec0, which correctly blocks the deprecated IPv6 site-local range. This matches the fix suggested in the previous review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 154 - 165, is_internal_v6 currently
checks embedded IPv4 via to_embedded_v4 and falls back to IPv6 segment tests;
ensure the deprecated site-local range fec0::/10 is blocked by keeping the
condition (segments[0] & 0xffc0) == 0xfec0 in the function is_internal_v6,
alongside the existing checks ((segments[0] & 0xfe00) == 0xfc00, (segments[0] &
0xffc0) == 0xfe80, addr.is_loopback(), addr.is_unspecified(),
addr.is_multicast()), and preserve the early return to is_internal_v4 for mapped
v4 addresses via is_internal_v4 so embedded IPv4 handling remains unchanged.

keep-nip46/src/bunker.rs (2)

319-324: Tests adapt to feature flag rather than asserting a fixed expectation.

These tests check validate_relay_url("ws://...") at runtime to decide the expected outcome. While pragmatic for a multi-config workspace, this means neither the ws://-rejected nor the ws://-accepted path is guaranteed to be exercised in any given test run. If allow-ws is accidentally enabled in a release profile, these tests won't catch it.

Consider using #[cfg(feature = "allow-ws")] and #[cfg(not(feature = "allow-ws"))] to write separate, unconditional assertions for each configuration, so both paths are always tested for their respective profile.

♻️ Example: split into feature-gated test branches

     #[test]
     fn test_parse_nostrconnect_rejects_non_wss_relay() {
         // ... URI construction ...
-        let ws_allowed = keep_core::relay::validate_relay_url("ws://relay.example.com").is_ok();
-        if ws_allowed {
-            assert!(parse_nostrconnect_uri(&uri).is_ok());
-        } else {
+        #[cfg(feature = "allow-ws")]
+        assert!(parse_nostrconnect_uri(&uri).is_ok());
+        #[cfg(not(feature = "allow-ws"))]
+        {
             assert!(parse_nostrconnect_uri(&uri).is_err());
         }
     }

Also applies to: 537-543

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-nip46/src/bunker.rs` around lines 319 - 324, Tests currently branch at
runtime using keep_core::relay::validate_relay_url("ws://...") to decide
expectations; instead split into two feature-gated test branches so each
configuration has an unconditional assertion: add one test block annotated
#[cfg(feature = "allow-ws")] that asserts parse_nostrconnect_uri(&uri).is_ok()
and another #[cfg(not(feature = "allow-ws"))] that asserts
parse_nostrconnect_uri(&uri).is_err(); reference the existing functions
validate_relay_url and parse_nostrconnect_uri when locating the test and remove
the runtime conditional so each build/profile exercises the correct path.

---

83-86: Asymmetric error handling for invalid relays between parse_nostrconnect_uri and parse_bunker_url.

parse_nostrconnect_uri silently drops invalid relays (line 84: validate_relay_url(&value).is_ok() as a filter) and only errors if zero valid relays remain, while parse_bunker_url fails immediately on the first invalid relay (line 145-146: validate_relay_url(&value).map_err(...)?). This means a bunker URL with one bad relay among several good ones will be rejected entirely, but the same mix in a nostrconnect URI would succeed with only the valid relays.

If this is intentional (e.g., nostrconnect comes from untrusted external clients, bunker URLs are user-configured), it's worth a brief doc comment on parse_bunker_url noting the strict behavior so future maintainers understand the design choice.

Also applies to: 141-147

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-nip46/src/bunker.rs` around lines 83 - 86, parse_bunker_url currently
errors on the first invalid relay via validate_relay_url(...).map_err(...)?
while parse_nostrconnect_uri filters out invalid relays and only errors if none
remain; decide which behavior is desired and either (A) make parse_bunker_url
permissive like parse_nostrconnect_uri by replacing the immediate map_err/? with
the same is_ok filter + normalize_relay_url push and only return an error if
relays.is_empty(), or (B) keep the strict behavior but add a clear doc comment
above parse_bunker_url stating it will reject the entire URL on any invalid
relay (reference validate_relay_url, parse_bunker_url, parse_nostrconnect_uri)
so future maintainers understand the intentional asymmetry.

keep-core/src/relay.rs (2)

435-504: Good test coverage for the new is_internal_ip function.

Tests cover IPv4 private ranges, IPv6 ULA/link-local, mapped IPv4, IPv4-compatible IPv6, multicast, and 0/8. Consider adding a test for fec0::1 (the site-local range added at line 161) in is_internal_ip_blocks_private to ensure end-to-end coverage of that fix.

📝 Add fec0::1 test case

         assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap()));
+        assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()));
         assert!(is_internal_ip(
             &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap()
         ));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 435 - 504, Add an assertion for the
site-local IPv6 address in the existing test is_internal_ip_blocks_private:
update that test to include
assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); so the
is_internal_ip function’s handling of the fec0::/10 site-local range (added
around the logic referenced at line 161) is exercised end-to-end.

---

154-165: Consider blocking 6to4 (2002::/16) addresses that embed private IPv4.

is_internal_v6 does not detect 6to4 addresses (2002::/16), which embed an IPv4 address in bits 16–47. An attacker could craft a DNS record returning e.g. 2002:c0a8:0101::1 (embeds 192.168.1.1). Since this function is specifically documented for post-DNS-resolution filtering, extracting and checking the embedded IPv4 from 6to4 addresses would close this bypass vector.

Similarly, 2001:db8::/32 (documentation prefix, analogous to TEST-NET) is not blocked.

🛡️ Proposed additions to is_internal_v6

 fn is_internal_v6(addr: &std::net::Ipv6Addr) -> bool {
     if let Some(v4) = to_embedded_v4(addr) {
         return is_internal_v4(v4);
     }
     let segments = addr.segments();
+    // 6to4 (2002::/16): extract embedded IPv4 from bits 16-47
+    if segments[0] == 0x2002 {
+        let v4 = std::net::Ipv4Addr::new(
+            (segments[1] >> 8) as u8,
+            segments[1] as u8,
+            (segments[2] >> 8) as u8,
+            segments[2] as u8,
+        );
+        return is_internal_v4(v4);
+    }
+    // Documentation prefix (RFC 3849)
+    if segments[0] == 0x2001 && segments[1] == 0x0db8 {
+        return true;
+    }
     (segments[0] & 0xfe00) == 0xfc00
         || (segments[0] & 0xffc0) == 0xfe80
         || (segments[0] & 0xffc0) == 0xfec0
         || addr.is_loopback()
         || addr.is_unspecified()
         || addr.is_multicast()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 154 - 165, is_internal_v6 currently
misses 6to4 (2002::/16) addresses and the documentation prefix 2001:db8::/32;
update is_internal_v6 to treat 6to4 specially by detecting segments[0] ==
0x2002, extracting the embedded IPv4 from segments[1] and segments[2] (combine
those 32 bits into an Ipv4Addr) and then call is_internal_v4 on that extracted
IPv4, and also add a check to return true for the documentation prefix by
matching segments[0] == 0x2001 && segments[1] == 0x0db8 so these ranges are
blocked post-DNS resolution.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-core/src/relay.rs (1)

435-452: ⚠️ Potential issue | 🟡 Minor

fec0::1 assertion missing from is_internal_ip_blocks_private

is_internal_v6 now correctly blocks fec0::/10 (line 161), but the test doesn't exercise this path.

✅ Proposed addition

         assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap()));
+        assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()));
         assert!(is_internal_ip(
             &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap()
         ));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 435 - 452, The test
is_internal_ip_blocks_private doesn't cover the IPv6 site-local range fec0::/10
even though is_internal_v6 blocks it; add an assertion in the
is_internal_ip_blocks_private test that calls
is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()) to ensure the fec0::1
address is considered internal (use the existing pattern of
assert!(is_internal_ip(...)) alongside the other IPv6 checks).

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-core/src/relay.rs (1)

154-165: ⚠️ Potential issue | 🟡 Minor

fec0::/10 is implemented but the companion test assertion is still missing

Line 161 correctly adds (segments[0] & 0xffc0) == 0xfec0 to cover the deprecated site-local range. However, neither is_internal_ip_blocks_private (lines 439–455) nor rejects_ipv6_internal (lines 309–317) contains an assertion for fec0::1 or wss://[fec0::1]/, leaving the guard untested.

✅ Suggested additions

     assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap()));
+    assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap()));
     assert!(is_internal_ip(
         &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap()
     ));

     assert!(validate_relay_url("wss://[fe80::1]/").is_err());
     assert!(validate_relay_url("wss://[ff02::1]/").is_err());
+    assert!(validate_relay_url("wss://[fec0::1]/").is_err());
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 154 - 165, Add test assertions to cover
the newly handled deprecated site-local IPv6 block fec0::/10: update the test
functions is_internal_ip_blocks_private and rejects_ipv6_internal to include an
assertion that is_internal_v6 parses/returns true for "fec0::1" (or equivalent
Ipv6Addr) and that the URL/reject logic rejects a websocket URL like
"wss://[fec0::1]/". Locate tests referencing is_internal_v6,
is_internal_ip_blocks_private, and rejects_ipv6_internal and add these two
assertions so the fec0::/10 branch (segments[0] & 0xffc0 == 0xfec0) is
exercised.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-core/src/relay.rs (1)

136-138: ⚠️ Potential issue | 🔴 Critical

Root cause of CI failures: tests don't account for allow-internal feature flag.

The allow-internal feature correctly bypasses the internal-host check for dev/testing scenarios, but the tests expecting internal addresses to be rejected (lines 284–293, 296–301, 304–306, 309–323, 332–338, 341–344, 346–349, 415–419, 422–435, 437–442, 480–490) don't have conditional logic to handle this feature flag. When CI runs with allow-internal enabled, all these tests fail because validate_relay_url returns Ok instead of Err.

The rejects_non_wss test (lines 260–264) demonstrates the correct pattern—add similar conditional handling to all internal-address rejection tests.

🔧 Example fix pattern (apply to all affected tests)

 #[test]
 fn rejects_internal_ipv4() {
+    if cfg!(feature = "allow-internal") {
+        // Internal addresses allowed when feature enabled
+        assert!(validate_relay_url("wss://127.0.0.1/").is_ok());
+        return;
+    }
     assert!(validate_relay_url("wss://127.0.0.1/").is_err());
     assert!(validate_relay_url("wss://127.0.0.2/").is_err());
     // ... rest of assertions
 }

Alternatively, use #[cfg_attr(feature = "allow-internal", ignore)] to skip these tests entirely when the feature is enabled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 136 - 138, Tests that assert internal
addresses are rejected fail when the allow-internal feature is enabled because
validate_relay_url will return Ok; update each failing test (the ones asserting
internal-host rejection) to follow the pattern used in rejects_non_wss by making
the assertion conditional on cfg!(not(feature = "allow-internal")) or annotate
the test with #[cfg_attr(feature = "allow-internal", ignore)] so they are
skipped when allow-internal is enabled; locate tests referencing
validate_relay_url and internal hosts (mentioned in the review) and apply one of
these two patterns consistently.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-frost-net/src/node/mod.rs (1)

328-337: Consider hoisting validator selection outside the loop.

Since ALLOW_INTERNAL_HOSTS is a compile-time constant, the validator function is the same for every iteration. Moving the selection outside the loop improves readability. The compiler likely optimizes this away, so this is purely a stylistic preference.

♻️ Suggested refactor

+        let validate: fn(&str) -> Result<(), String> = if ALLOW_INTERNAL_HOSTS {
+            validate_relay_url_allow_internal
+        } else {
+            validate_relay_url
+        };
         for relay in &relays {
-            let validate = if ALLOW_INTERNAL_HOSTS {
-                validate_relay_url_allow_internal
-            } else {
-                validate_relay_url
-            };
             validate(relay).map_err(|e| {
                 FrostNetError::Transport(format!("Rejected relay URL {relay}: {e}"))
             })?;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-frost-net/src/node/mod.rs` around lines 328 - 337, Move the validator
selection out of the loop: choose the function once based on the compile-time
constant ALLOW_INTERNAL_HOSTS (select either validate_relay_url_allow_internal
or validate_relay_url) before iterating over relays, then call the selected
validator inside the for relay in &relays loop and map errors to
FrostNetError::Transport as before; this removes repeated branching per
iteration and improves readability while keeping the existing
validate(...).map_err(...) behavior.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

keep-desktop/src/app.rs (1)

348-355: Consider falling back to defaults when all saved bunker relays are filtered out.

If a user's saved bunker relay config contains only URLs that fail validation (e.g., internal addresses now blocked), filter_valid_relays returns an empty list. The unwrap_or_else(default_bunker_relays) only applies when deserialization fails, not when filtering empties a valid list.

This edge case might leave users with no bunker relays until they manually reconfigure. A fallback could improve UX:

🔧 Optional: Fall back to defaults when filtered list is empty

 fn load_bunker_relays(keep_path: &std::path::Path) -> Vec<String> {
     let path = bunker_relay_config_path(keep_path);
     let urls: Vec<String> = std::fs::read_to_string(&path)
         .ok()
         .and_then(|s| serde_json::from_str(&s).ok())
         .unwrap_or_else(default_bunker_relays);
-    filter_valid_relays(urls)
+    let filtered = filter_valid_relays(urls);
+    if filtered.is_empty() {
+        tracing::warn!("All saved bunker relays invalid, using defaults");
+        default_bunker_relays()
+    } else {
+        filtered
+    }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-desktop/src/app.rs` around lines 348 - 355, The current
load_bunker_relays reads JSON and falls back to default_bunker_relays only on
deserialization error, but after calling filter_valid_relays it may return an
empty Vec when all saved URLs are invalid; update load_bunker_relays so that
after deserializing and calling filter_valid_relays(urls) you check if the
resulting Vec is empty and, if so, return default_bunker_relays() instead;
reference functions: load_bunker_relays, bunker_relay_config_path,
filter_valid_relays, and default_bunker_relays to locate and implement this
empty-result fallback.

keep-core/src/relay.rs (1)

480-497: Add test coverage for deprecated site-local fec0::/10 range.

The is_internal_v6 function now correctly includes the fec0::/10 site-local check (line 178), but this test doesn't exercise that path. Adding fec0::1 would ensure the deprecated-but-exploitable range is covered.

✅ Suggested test addition

         assert!(is_internal_ip(&"fc00::1".parse::<IpAddr>().unwrap()));
         assert!(is_internal_ip(&"fe80::1".parse::<IpAddr>().unwrap()));
+        assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); // deprecated site-local
         assert!(is_internal_ip(
             &"::ffff:127.0.0.1".parse::<IpAddr>().unwrap()
         ));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@keep-core/src/relay.rs` around lines 480 - 497, The test is missing coverage
for the deprecated IPv6 site-local range fec0::/10; update the is_internal_ip
tests by adding an assertion that parses "fec0::1" into std::net::IpAddr and
passes it to is_internal_ip (or exercise is_internal_v6 via is_internal_ip) to
assert true so the fec0::/10 check is covered; locate the test function
is_internal_ip_blocks_private and add the
assert!(is_internal_ip(&"fec0::1".parse::<IpAddr>().unwrap())); line.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro